Skip to content

storage: Passphrase dialog fields can reveal the text#18906

Merged
martinpitt merged 1 commit intocockpit-project:mainfrom
mvollmer:storage-password-reveal
Jun 20, 2023
Merged

storage: Passphrase dialog fields can reveal the text#18906
martinpitt merged 1 commit intocockpit-project:mainfrom
mvollmer:storage-password-reveal

Conversation

@mvollmer
Copy link
Member

@mvollmer mvollmer commented Jun 8, 2023

No description provided.

@mvollmer mvollmer force-pushed the storage-password-reveal branch 2 times, most recently from 544e3ca to 36fdfe4 Compare June 9, 2023 09:37
@martinpitt
Copy link
Member

Is this some new recommendation from PatternFly or somewhere else? This lacks a reference and a justification.

I don't like this. It's IMNSHO very useful to confirm that you can reproduce the passphrase at least once, independent of the revealing. And not let this be done by some Yubikey, a cat walking over your keyboard, or fat-fingering the result.

Also, it's conflicty now, apparently some other pixel update landed in between.

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@mvollmer
Copy link
Member Author

Is this some new recommendation from PatternFly or somewhere else? This lacks a reference and a justification.

I thought this is what we are doing in Cockpit, and the Storage page just has to catch up. The sospage uses this pattern (single text entry with reveal functionality for a new password), but I can't find any other...

@garrett, can you clarify? Should all password fields in Cockpit have a reveal functionality? Should we have a single text input for new passwords, or two?

I know of these articles:

@martinpitt
Copy link
Member

To clarify: I welcome the "reveal" functionality. I don't like the "drop confirmation input" part.

@mvollmer mvollmer force-pushed the storage-password-reveal branch from 36fdfe4 to 5e72117 Compare June 13, 2023 09:43
@garrett
Copy link
Member

garrett commented Jun 13, 2023

Nielsen Norman Group is huge in the UI/UX design space (they've literally written several authoritative books on the subject). Here are several more posts from others about removing the password confirmation:

And as far as PatternFly is concerned: None of PatternFly's password creation examples have a repeat. But they do (usually) have a reveal.

So: What's the way our users will be creating and storing these passwords?

  • Are they copy/pasting from somewhere else?
  • Are they typing in what is on a sheet of paper?
  • Are they using a password manager (even a browser) to generate the password?

A confirmation prompt is not needed for any of these examples and actively gets in the way.

Perhaps, if they are making something up themselves it might be useful. But even then, they might type the wrong thing twice. Or have caps lock on. Or number lock (if they're entering keys on the keypad). Or sticky keys might interfere, if they're using accessibility features.

Having a second entry means more busywork. It's also not actually used for confirmation! If we were actually having a second one for confirmation, then we'd actually ask for the password after creation and checking to see if it was unlocked, right? In other words: having a "confirmation" field doesn't actually do anything at all to validate the password to make sure it's correct.


Anyway, while it's been the general recommendation to drop the confirmation/repeat/verify password field for around the past decade and a half, it's not commonplace in a lot of OS designs yet.

GNOME current user creation:
Screenshot from 2023-06-13 11-36-08

GNOME next user creation:

add-user
https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/users/add-user.png

macOS Monterey (previous version) user creation:

Screenshot from 2023-06-13 11-31-08

Windows 11 local user creation:

image

However, these are all user account creation UIs, not disk encryption.

macOS Ventura (latest version) has info here: https://support.apple.com/guide/disk-utility/encrypt-protect-a-storage-device-password-dskutl35612/mac which says, "8. Enter and verify a password, then click Choose." (emphasis added).

Here's GNOME's current UI from GNOME Disks for creating a LUKS volume:

image

(I haven't found anything about it in the various redesigns @ https://gitlab.gnome.org/GNOME/gnome-disk-utility/-/issues/28#note_305987 or https://gitlab.gnome.org/Teams/Design/app-mockups/-/blob/master/disks/disks.png or the recent tracking issue @ https://gitlab.gnome.org/GNOME/gnome-disk-utility/-/issues/274)

So: What has been a "best practice" for the web for well over a decade still hasn't made it into user & disk password creation UI in OSes yet.

Now: I wonder what other PF-using RH products do. (I seem to recall OpenShift not having a repeat, but I couldn't find the screenshot I took a while ago during a demo call.)

@garrett
Copy link
Member

garrett commented Jun 13, 2023

Summary: While dropping the verify is generally accepted design advice for the web (and there are several articles and studies showing that it is not needed), it's not in practice everywhere (as seen from the OS design screenshots I posted above). And Pitti doesn't agree with it.

We should definitely get the view password in regardless and not block on dropping the repeat password.

@martinpitt
Copy link
Member

Thanks @garrett for the references and research! To clarify, this wasn't a veto, just a strong feeling that I don't like it.

Are they copy/pasting from somewhere else?

In that case it's cheap to paste it twice.

Are they typing in what is on a sheet of paper?

That's exactly the case that I'm concerned about -- if your password is so complicated that you make an accidental typo, it could have major consequences if you don't actually type the password that you intend to. And then after a reboot you can't access your files any more. Proving that the user is able to type it at least twice reduces the chance of that happening.

Are they using a password manager (even a browser) to generate the password?

Not plausible for an encrypted file system, these are usually unlocked during boot, aren't they? (Or with clevis/tang)

If you feel strongly about dropping it, I don't veto it -- I just wanted to put this into the discussion that I still see value in it.

@mvollmer mvollmer force-pushed the storage-password-reveal branch 2 times, most recently from 0452c99 to 2658277 Compare June 13, 2023 13:07
@garrett
Copy link
Member

garrett commented Jun 14, 2023

If you feel strongly about dropping it, I don't veto it -- I just wanted to put this into the discussion that I still see value in it.

It's fine if we don't, especially for disk encryption. We might want to consider dropping it elsewhere though (user accounts, especially ones that aren't the logged in user of Cockpit)... passwords for accounts and other things like disk encryption are different use cases.

I was surprised that GNOME, macOS, and Windows all have a second prompt in all of them, for both user and disk.

KDE does this too, BTW:

Screenshot from 2023-06-14 10-21-32
Screenshot from 2023-06-14 10-23-26


Therefore, I'm fine with keeping this to passphrase reveal only. We can discuss a possible removal of repeat/verify in another place.

@mvollmer mvollmer force-pushed the storage-password-reveal branch from 2658277 to c821894 Compare June 15, 2023 12:47
@mvollmer
Copy link
Member Author

Therefore, I'm fine with keeping this to passphrase reveal only.

Thanks for figuring this all out! I have updated the PR to only add the reveal functionality.

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If by some magic new_passphrase was intended,then I'd like to understand it. Thanks!

@mvollmer mvollmer force-pushed the storage-password-reveal branch from c821894 to 5b7c7f1 Compare June 16, 2023 10:44
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Danke!

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, still needs a pixel update, see the failed tests.

And mark entries for new passwords accordingly, in the hope that
browsers will do the right thing.
@mvollmer mvollmer force-pushed the storage-password-reveal branch from 5b7c7f1 to 6a3e725 Compare June 19, 2023 13:46
@mvollmer mvollmer requested a review from martinpitt June 19, 2023 13:46
PassInput("new_passphrase", _("New passphrase"),
{ validate: val => !val.length && _("Passphrase cannot be empty") }),
{
validate: val => !val.length && _("Passphrase cannot be empty"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

PassInput("new_passphrase2", _("Repeat passphrase"),
{ validate: (val, vals) => vals.new_passphrase.length && vals.new_passphrase != val && _("Passphrases do not match") })
{
validate: (val, vals) => vals.new_passphrase.length && vals.new_passphrase != val && _("Passphrases do not match"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

@martinpitt martinpitt merged commit a622cea into cockpit-project:main Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants